-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Video Filters and Save Video #1306
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all: Awesome job! Unfortunately it is very hard to see a diff because of all the reformatted code, could you please keep the current formatting?
@n1ru4l Thanks! Yeah not a problem. AppCode kept reformatting it for me, but it should be good to go now. |
Hi @nickgzzjr, thanks for submitting this. Having save functionality will be great, a lot of people want it! I did a few minor updates to the docs and function names. Imo, calling is save() is ok since you cover that in the docs it doesn't need to be saveAsync(). On the filters, can we name them the same as they are on iOS (Noir, Sepia, Posterize, etc)? I'm a big fan of not modifying APIs so that if someone is looking at the iOS docs to figure out which filter they want, they don't have to translate names. I'd also like to support as many of the filters on the list as possible. I would put all of the filters in a FilterType.js file so that you just go FilterType.SEPIA like you can with TextTrackType. I would also add a link to the iOS docs so developers can see what the filters look like: For saving, that needs a lot of documentation for it to be useful to other developers. A couple questions I'd have if I was using the API:
Can you update the docs to cover this and any other implementation details that you think people need to know. |
Hi @cobarx, thanks so much for you feedback, and your doc changes.
I need to do further research and testing when saving videos that are buffering and cached. My guess is that it works just fine since it grabs the video from the player asset item. |
@cobarx I can confirm that saving cached videos works, as well as videos that are buffering. I have updated the docs to reflect my findings. I modified the video-caching example to test. Let me know if there are any other suggestions. |
@nickgzzjr Thanks for taking the time to make the changes I requested. I finally had a chance to test the filters, those are very cool! I ran into a couple issues with the filters. First, when I play a video with the filter turned on in the iOS Simulator, it's very jerky and is maxing out the CPU on my machine. Activity Monitor shows 200% cpu vs. 30% cpu with it off. Have you seen this when testing? Can you see if you can figure out what might be causing this? Second, the filters don't appear to work for HLS playlists. If I play an MP4, the filter works but for HLS nothing happens. Here is a sample stream you can use for testing: |
@cobarx I am glad you liked them :) The way filters work is that every video frame is processed with an image filter. This is why it uses so much CPU. I have only experienced "jerkyness" on the simulator but not an actual device. One workaround is to save the video first then load the output. Unfortunately, I was not able to get the HLS playlist to work. I went ahead and added notes to the docs on these 2 things. I also modified where the filter gets applied as it was conflicting with the repeat functionality. It's a known issue when using videoComposition and seeking. It would be nice if you could test the filters one last time with the repeat option as well. |
|
Video Filters and Save Video (rebased from commit 5e684d4)
I added the ability to set video filters to the video player. This can be passed as a prop to the Component.
I also added the ability to save the video with the current filter using a saveAsync method that returns a promise.
I am open to changes and suggestions.